Add ability to evict a container #3549

marcov · 2019-07-10T17:35:42Z

Add ability to evict a container when it becomes unusable. This may
happen when the host setup changes after a container creation, making it
impossible for that container to be used or removed.

This may happen when you create a container with a specific runtime in libpod.conf, and later the runtime name is changed in the the conf, with outcome:

$ sudo podman run -d --runtime foo-runtime alpine sleep 12345
add950497c58b4004aee3a6077658615882f8487f9dca4a337fad14c01fb4077

$ sudo sed -i "s/foo-runtime/bar-runtime/" /etc/containers/libpod.conf

$ sudo podman ps -a
ERRO[0000] Error retrieving container add950497c58b4004aee3a6077658615882f8487f9dca4a337fad14c01fb4077 from the database: cannot find OCI runtime "foo-runtime" for container add950497c58b4004aee3a6077658615882f8487f9dca4a337fad14c01fb4077: runtime not available in the current configuration
CONTAINER ID  IMAGE                        COMMAND               CREATED      STATUS                     PORTS                                          NAMES
50c843e037fe  localhost/cups-epson:latest  cupsd -f              2 hours ago  Exited (0) 31 minutes ago  0.0.0.0:6631->631/tcp, 0.0.0.0:3000->3000/tcp  suspicious_germain
c684f0d469f2  k8s.gcr.io/pause:3.1                               9 days ago   Exited (0) 31 minutes ago  0.0.0.0:6631->631/tcp, 0.0.0.0:3000->3000/tcp  e812f6be094a-infra

$ sudo podman rm -ff ad
add950497c58b4004aee3a6077658615882f8487f9dca4a337fad14c01fb4077

openshift-ci-robot · 2019-07-10T17:36:03Z

Hi @marcov. Thanks for your PR.

I'm waiting for a containers or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

marcov · 2019-07-11T17:03:15Z

@mheon I'd like your feedback on this, to see if it is going in the right direction.

rh-atomic-bot · 2019-07-11T19:38:26Z

☔ The latest upstream changes (presumably #3470) made this pull request unmergeable. Please resolve the merge conflicts.

mheon · 2019-07-11T20:00:35Z

libpod/boltdb_state_internal.go

+// Evicts a container from the DB
+// If the config specifies a pod, the container is treated as belonging to a pod,
+// and will be removed from the pod as well
+func (s *BoltState) evictContainer(ctrID []byte, config *ContainerConfig, depCtrs []string, tx *bolt.Tx) error {


Hmmm. I think we can reuse RemoveContainer() here - it doesn't actually need anything in the container state.

We can make a Container struct with a ContainerConfig we get out of GetContainerConfig, set state to an empty struct to avoid potential dereference segfaults, and pass it into RemoveContainer

That makes sense, I have done some more refactoring and removed a good part of the duplicated code.

marcov · 2019-07-12T11:34:44Z

@mheon unrelated to this PR: here c is a pointer, and shadowing does not make sense:
https://github.com/containers/libpod/blob/6f3e7f7eccdfed03d3d617a9040d5e0b844ea637/pkg/adapter/containers.go#L212-L213

rhatdan · 2019-07-12T19:53:38Z

/ok-to-test

marcov · 2019-08-19T14:02:47Z

@vrothberg, if I do like you said, then how can I call iopodman.EvictContainer here?

https://github.com/containers/libpod/blob/128a1707f7d9e8df648a6ba353bfa16b9bef2431/pkg/adapter/containers_remote.go#L323-L339

mheon · 2019-08-19T14:22:01Z

Why are we trying to force the Varlink API to follow CLI conventions? These are separate operations and the API should treat them as such, even if we decide to conflate in the command line.

…

On Mon, Aug 19, 2019, 10:03 Marco Vedovati ***@***.***> wrote: @vrothberg <https://github.com/vrothberg>, if I do like you said, then how can I call iopodman.EvictContainer here? https://github.com/containers/libpod/blob/128a1707f7d9e8df648a6ba353bfa16b9bef2431/pkg/adapter/containers_remote.go#L323-L339 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3549>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB3AOCG455MHJDMG2GUSW4DQFKR25ANCNFSM4H7RUQJQ> .

rhatdan · 2019-09-06T17:54:25Z

Any update on this PR?

marcov · 2019-09-06T17:55:57Z

I am still willing to have this merged, just let me know if there's something to change.

mheon · 2019-09-06T17:59:38Z

I think we're relatively close - I'll do another review pass on Monday.

giuseppe · 2019-09-16T12:51:23Z

@marcov could you please rebase this PR?

marcov · 2019-09-16T18:38:29Z

Rebased

vrothberg · 2019-09-24T12:31:14Z

Tests aren't happy yet. @marcov, once it's green we can merge it :)

@mheon, can you do another review?

marcov · 2019-09-24T14:13:26Z

Failures looked unrelated at a first glance. I've rebased and let's see...

vrothberg · 2019-09-25T11:55:50Z

@marcov, the error seems to be introduced with this PR. Running go test -v github.com/containers/libpod/libpod will report a breaking unit test.

marcov · 2019-09-25T16:35:01Z

Thank you @vrothberg, a mere glance was not enough 😆

Add ability to evict a container when it becomes unusable. This may happen when the host setup changes after a container creation, making it impossible for that container to be used or removed. Evicting a container is done using the `rm --force` command. Signed-off-by: Marco Vedovati <[email protected]>

marcov · 2019-09-26T14:21:18Z

Ok, tests now are better. I don't think the only test failing is related to this PR (hopefully I'm not wrong again)

rhatdan · 2019-09-29T13:55:50Z

Ok this needs a review by @mheon and then we can merge.

mheon · 2019-10-04T13:53:08Z

I think we have some unsettled debate about how and when to call this - but that can happen separately.
/lgtm

haircommander · 2019-10-04T14:15:13Z

(still needs /approve)

mheon · 2019-10-04T14:17:31Z

Oops
/approve

openshift-ci-robot · 2019-10-04T14:17:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marcov, mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mheon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2019

openshift-ci-robot requested review from giuseppe and TomSweeneyRedHat July 10, 2019 17:35

openshift-ci-robot added size/XL needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 10, 2019

marcov mentioned this pull request Jul 10, 2019

Allow force removing containers with unavailable OCI runtime #3502

Closed

marcov force-pushed the evict-container branch 3 times, most recently from 169147a to ecd82b2 Compare July 11, 2019 11:20

marcov mentioned this pull request Jul 11, 2019

rm of a container in a pod using a volume breaks podman #3556

Closed

marcov force-pushed the evict-container branch 4 times, most recently from 9e00885 to fa7b9b3 Compare July 11, 2019 17:01

mheon reviewed Jul 11, 2019

View reviewed changes

marcov force-pushed the evict-container branch from fa7b9b3 to acd983c Compare July 12, 2019 11:01

openshift-ci-robot added size/L and removed size/XL labels Jul 12, 2019

marcov force-pushed the evict-container branch from acd983c to c940881 Compare July 12, 2019 11:11

marcov changed the title ~~[WIP] Add ability to evict a container~~ Add ability to evict a container Jul 12, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 12, 2019

openshift-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 12, 2019

marcov force-pushed the evict-container branch 2 times, most recently from f811890 to 0c39d07 Compare July 15, 2019 10:55

giuseppe mentioned this pull request Sep 16, 2019

Container left semi-alive after removal failed with error unmounting container: layer is not mounted #4033

Closed

marcov force-pushed the evict-container branch from 128a170 to 081d7b1 Compare September 16, 2019 18:38

marcov force-pushed the evict-container branch from 081d7b1 to 8fb722a Compare September 16, 2019 18:49

marcov force-pushed the evict-container branch from 8fb722a to f862dfa Compare September 24, 2019 14:12

marcov force-pushed the evict-container branch from f862dfa to 95e39de Compare September 25, 2019 16:34

marcov force-pushed the evict-container branch from 95e39de to dacbc5b Compare September 25, 2019 17:44

openshift-ci-robot assigned mheon Oct 4, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2019

openshift-merge-robot merged commit c9e936a into containers:master Oct 4, 2019

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to evict a container #3549

Add ability to evict a container #3549

marcov commented Jul 10, 2019

openshift-ci-robot commented Jul 10, 2019

marcov commented Jul 11, 2019

rh-atomic-bot commented Jul 11, 2019

mheon Jul 11, 2019

marcov Jul 12, 2019

marcov commented Jul 12, 2019

rhatdan commented Jul 12, 2019

marcov commented Aug 19, 2019

mheon commented Aug 19, 2019 via email

rhatdan commented Sep 6, 2019

marcov commented Sep 6, 2019

mheon commented Sep 6, 2019

giuseppe commented Sep 16, 2019

marcov commented Sep 16, 2019

vrothberg commented Sep 24, 2019

marcov commented Sep 24, 2019 •

edited

Loading

vrothberg commented Sep 25, 2019

marcov commented Sep 25, 2019

marcov commented Sep 26, 2019

rhatdan commented Sep 29, 2019

mheon commented Oct 4, 2019

haircommander commented Oct 4, 2019

mheon commented Oct 4, 2019

openshift-ci-robot commented Oct 4, 2019

Add ability to evict a container #3549

Add ability to evict a container #3549

Conversation

marcov commented Jul 10, 2019

openshift-ci-robot commented Jul 10, 2019

marcov commented Jul 11, 2019

rh-atomic-bot commented Jul 11, 2019

mheon Jul 11, 2019

Choose a reason for hiding this comment

marcov Jul 12, 2019

Choose a reason for hiding this comment

marcov commented Jul 12, 2019

rhatdan commented Jul 12, 2019

marcov commented Aug 19, 2019

mheon commented Aug 19, 2019 via email

rhatdan commented Sep 6, 2019

marcov commented Sep 6, 2019

mheon commented Sep 6, 2019

giuseppe commented Sep 16, 2019

marcov commented Sep 16, 2019

vrothberg commented Sep 24, 2019

marcov commented Sep 24, 2019 • edited Loading

vrothberg commented Sep 25, 2019

marcov commented Sep 25, 2019

marcov commented Sep 26, 2019

rhatdan commented Sep 29, 2019

mheon commented Oct 4, 2019

haircommander commented Oct 4, 2019

mheon commented Oct 4, 2019

openshift-ci-robot commented Oct 4, 2019

marcov commented Sep 24, 2019 •

edited

Loading